Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(tracing): set attribute http.status_code when no route entry matched #11711

Closed
wants to merge 5 commits into from

Conversation

liverpool8056
Copy link
Contributor

Summary

set attribute http.status_code in span when no route entry matched

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Full changelog

  • [Implement ...]

Issue reference

FTI-5407

@@ -1145,6 +1145,7 @@ return {
if not match_t then
-- tracing
if span then
span:set_attribute("http.status_code", 404)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is adding the attribute to the router span, which is not always enabled, I think we should add it to the root (kong) span as well, which is the span that normally contains this information, see here.

Also, is this the same (or a very similar) issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, Yeah, thanks! Being an attribute of root span is much more reasonable, and it seem optional to be added to router span, and I'd like just add it to root span.

same purpose of recording the status as #11406 , but it can't cover the issue in this case.

Copy link
Member

@samugi samugi Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liverpool8056 unless I'm missing something, I think #11406 solves this problem too. I just ran your test without this fix and it's passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just verified the effectiveness of #11406 too. And it reminds that kong.response.exit() doesn't interrupt the current request, and the remaining phases are still gonna to be executed, so it's better to mark the attribute of root span at later phase as possible as we can.

Let me close this one.

@liverpool8056 liverpool8056 force-pushed the fix/set-http-status-when-404 branch from 2d7ec16 to 3678fec Compare October 9, 2023 09:08
@liverpool8056 liverpool8056 force-pushed the fix/set-http-status-when-404 branch from 3678fec to c1d6ca9 Compare October 10, 2023 02:34
@liverpool8056 liverpool8056 requested a review from samugi October 10, 2023 06:41
Copy link
Member

@samugi samugi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks to be already resolved by #11406

@liverpool8056 liverpool8056 deleted the fix/set-http-status-when-404 branch January 9, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants